Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Guo Jingxue] iP #237

Open
wants to merge 35 commits into
base: master
Choose a base branch
from
Open

Conversation

jingxueguo
Copy link

No description provided.

Copy link

@ming-00 ming-00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Few nits to work out, and the main method may be way too long.

while (sc1.hasNextLine()) {
data = sc1.nextLine();
String[] dataSplit = data.split(" \\| ");
switch (dataSplit[0]) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should check the switch identation


@Override
public String toString() {

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid leaving spaces!

+ "| |_| | |_| | < __/\n"
+ "|____/ \\__,_|_|\\_\\___|\n";
System.out.println("Hello from\n" + logo);
int counter = 0;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should avoid overly long methods that take up more than the space in a computer screen

Copy link

@garyljj garyljj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Overall looks not bad!
No problem with any variable names 👍 But perhaps can private attributes for better encapsulation.
And also, since main method is pretty long, might be good idea to use blank lines to split up logical sections for easier readability!

import java.time.format.DateTimeFormatter;

public class Deadlines extends Task {
LocalDate deadLine;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to private attributes for encapsulation 👍

@@ -0,0 +1,11 @@
public class ToDos extends Task {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps use singular form (ToDo) since plural form usually represents a collection of objects, eg. List

Comment on lines 58 to 60
switch (inputSplit[0]){
case "done":
if (inputSplit.length < 2){
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor but remember to leave white space before "{" just for consistency and readability 😄


@Override
public String toString() {
return "[D] " + super.toString() + "(by: " + deadLine.format(DateTimeFormatter.ofPattern("MMM d yyyy")) + ")";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you could try having this over two lines?

Quoting the java coding standards:

Try to keep line length shorter than 110 characters (soft limit). But it is OK to exceed the limit slightly (hard limit: 120 chars).

I think the line currently is 110+ characters, but it may improve readability slightly if placed over two lines.

this.eventName = eventName;
this.eventType = eventType;
}
public void setDone() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to add an empty line between the class constructor and the method, so that there is better readability?

String time;

public Events(boolean isDone, String eventName, String time) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see an empty line here, but not in your constructors for your other classes. Perhaps you could remove this empty line to make it consistent with the rest?

@hengyiqun
Copy link

Hi, very clear code overall!
Just a few nits to work on!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants